This PERFLAB_PLUGIN_DIR_URL is plugin_dir_url( PERFLAB_MAIN_FILE ), but I believe here it would be better to use plugins_url():

The reason is that this function allows for filtering as needed.

@westonruter I don't think we should randomly change it here though. If not using plugins_url() leads to a problem, let's discuss that in a separate issue and implement properly if needed.

I've filed #1647 for this.

If my suggestion above is accepted, this can then be removed.

See above, let's discuss separately, it's not really relevant for this enhancement.

At least let's avoid introducing a new constant in this PR.

That's fair, we can simply use plugin_dir_url( PERFLAB_MAIN_FILE ) in the wp_enqueue_script() call directly. I'm personally not a fan of having constants for all this either, particularly as the values are filterable, like you're saying.

I've filed #1647 for the filtering.

What if someone clicks this button while another request is currently in flight? Should it short-circuit so as to avoid duplicating requests?

I think as the request could have been half finished we should not short-circuit the request. I think it will be better to just disable it until the request is finished. But as the click event is on the anchor tag we will have to add pointer-events: none CSS to the anchor link using JavaScript to disable it.

The pointer-events style wouldn't address the issue of the button being activated by keyboard.

As the click event is on the anchor tag and not button we will have to add some other work around for disabling on keyboard one is setting its tabindex attribute to -1.

I tested this but if user is already focused then tabindex will not work. One other way is to add a attribute like aria-disabled and then check on click event if it has that attribute then return from the execution. Should I add tabindex and aria-disabled both?

You could add the disabled attribute, but that wouldn't be good because it would lose the tab focus for users navigating with the keyboard. Instead of this, WordPress has style rules for when you add the disabled class to a button so that it will look disabled. Adding aria-disabled here too would be good. But then for how to detect whether request is currently in progress, you could just short-circuit the click handler if the button has the updating-message class, or if it has the disabled class, or if it has the aria-disabled attribute.

Does it make sense to make jQuery a dependency? Would plain JS not be feasible?

+1, it would be great to use vanilla JavaScript rather than relying on jQuery, to set a good example for performance.

In the demo video you showed that this notice is causing layout shifts, resulting in you accidentally clicking the title of a plugin instead of the Activate button. I don't think an admin notice is even needed here, because the button transitions from Activate to Activating... to Active.

Related to @westonruter's previous feedback, it would be great not to use jQuery for the functionality, but rather vanilla JavaScript (e.g. fetch() function).

Do we need these enqueue now? as it already added through new script dependancy.

We could remove it as it is not being used anywhere other than the AJAX script as it already added in its dependancy. Should I remove it then?

Actually, in #1646 (comment) I just posed this change as a possibility but then recommended against it since it would only account for Speculative Loading. It wouldn't account for other plugins that have delayed bootstrapping. So to address the problem of accessing the settings link after a plugin is activated, I think the best solution is to make another request specifically to an endpoint that exposes the settings link URLs.

Aside: I think the REST API would be better for this instead of admin-ajax.

Why not use the REST API? You could just use wp.apiFetch then without having to export perflabPluginActivateAjaxData to the frontend.

You could add the disabled attribute, but that wouldn't be good because it would lose the tab focus for users navigating with the keyboard. Instead of this, WordPress has style rules for when you add the disabled class to a button so that it will look disabled. Adding aria-disabled here too would be good. But then for how to detect whether request is currently in progress, you could just short-circuit the click handler if the button has the updating-message class, or if it has the disabled class, or if it has the aria-disabled attribute.

As noted in a comment below, this can use wp-api-fetch instead if the REST API is used.

Maybe just update the existing button instead of replace the button? By keeping the same element in place, users with a keyboard won't have the focus removed from the document.

The target is actually <a> tag ( because it should be able to work even if JavaScript is disabled ) and to match the mark up of loaded page which is disabled button because of that I am replacing the target <a> tag with button.

I think the disabled class works with a link as well. I think you can keep it as a link instead of replacing it with a button.

Maybe this should throw an error because the same logic here is located in the catch block.

Nit: the hyphen can be removed since it is only used with @param to make it more readable. It's not seen in the docs for @return.

Since this endpoint is not a noun, it may make more sense as /plugin:activate. See for example in Optimization Detective:

Indeed this is not a great resource URI for a REST URL, as it's not truly RESTful. REST endpoints should be centered around a resource, in this case a plugin.

But even better would be something like: /plugins/<slug>:activate. This would also be more intuitive in that the required parameter slug would be directly in the route path.

@felixarntz I was just curious what should be the criteria for deciding whether to add the slug to the URL itself or include it in the body.

@b1ink0 It's one of the best practices for designing RESTful APIs, they should be centered around resources. See for example https://google.aip.dev/121 and https://google.aip.dev/122. (While these links are for Google's best practices, the best practices are not really specific to Google.)

By centering the API endpoints and their names around resources and their hierarchies, you future-proof the API to remain consistent and intuitive even when you add more and more features to it.

Here, the resource is a plugin. That's why using something like /plugins/<slug> as foundation for the routes is a good starting point. Such an endpoint could be used to get data for a specific plugin, or update data for a plugin for example.

Activating a plugin is not one of the CRUD operations though, so that's where a custom method is needed. See https://google.aip.dev/136 in that regard: That's how /plugins/<slug>:activate makes sense.

For example, if we later needed a method to deactivate a plugin, that would be straightforward via /plugins/<slug>:deactivate. If we used /activate-plugin, this would need to be /deactivate-plugin. That's still somewhat consistent but not centered around the resource and less intuitive. For example, nothing in the name tells you that you have to provide a slug. Additionally, even if you guessed that some identifier for the plugin would be needed, you wouldn't know whether it needs to be a parameter called slug or id or identifier for example. The resource identifier being part of the endpoint path makes that easier to work with.

@westonruter Following up on the other thread (since it probably makes sense to center that conversation about the endpoint naming): Per https://google.aip.dev/122, I think what would make most sense for the settings URL would be to simply be a value on the object returned by GET /plugins/<slug>.

A settings URL is not part of a collection, because a plugin does not have multiple settings URL. So GET /plugins/<slug>/settings-url would not be right. Because it's not an action (as you previously stated), GET /plugins/<slug>:settings-url wouldn't be right either.

The settings URL is one scalar piece of data for a plugin, so it makes most sense to be exposed on a plugin object. Obviously for this PR we don't care about creating a fully fledged GET /plugins/<slug> endpoint, but that's fine. We can simply for now return an object that e.g. only has slug and settingsUrl properties. The schema for such an object could be extended in the future in case we ever found a need for it, without backward compatibility breaks.

So I think going with POST /plugins/<slug>:activate and GET /plugins/<slug> (the latter of which return an object that includes the settings URL) is best in line with the API design guidelines.

@felixarntz Additionally, there is also already a way to activate a plugin using the REST API by setting the status field on the entity to active, without resorting to a special endpoint naming scheme: https://developer.wordpress.org/rest-api/reference/plugins/#update-a-plugin

However, reusing this endpoint is more complicated since we also automatically install the plugin prior to activation, as well as installing and activating any dependencies.

So maybe we should keep our own endpoint for installation and activation but instead of calling it plugin:activate we call it feature:activate to differentiate it from general plugin activation, as these plugins represent performance features.

In the end, this API is almost guaranteed to be used 100% internally and so we don't really need to stress about it being getting it right forever.

I don't think the existing plugin endpoints in Core are a problem. We use a different namespace specific to Performance Lab, so I wouldn't worry about that. It's IMO not unreasonable to provide custom endpoints for similar use-cases under a different namespace when the Core endpoints do not satisfy the requirements we have.

Regarding updating a status field to active, that's how they chose to build it, but doesn't seem that great to me per https://google.aip.dev/216.

That said, I'd be okay using the terminology "features" instead of "plugins" for our endpoints, since that's also what we use in the UI, and I agree it can help differentiate.

So let's go with POST /features/<slug>:activate and GET /features/<slug> then?

Where GET /features/<slug> returns an object with one key, settingsUrl which may be either a string or null, right? Sounds good.

Exactly.

If we like, we could also include e.g. slug in the response object as that's simple enough and clarifies the intent of the endpoint even more (that it's supposed to return plugin information, so not necessarily just the settings URL).

This can be eliminated in favor of adding the validation to the REST API arg.

This code isn't needed because the validation is handed by the REST API, per above.

This is not needed because the REST API schema enforces that the slug parameter will be present.